Add option to sanitize annotation inputs #7874

rikatz · 2021-11-02T20:49:39Z

This PR adds two new items in ConfigMap:

annotation-value-word-blocklist

Based on this, Ingress will drop objects that contians annotations with those words or chars. I'm still thinking about dropping the chars and keeping only the word validation (as chars are just words with 1 character...)

TODO:

Docs
e2e test for webhook
Deciding if we should drop the char validation

k8s-ci-robot · 2021-11-02T20:50:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

strongjz · 2021-11-02T20:51:37Z

internal/ingress/controller/config/config.go

@@ -754,6 +762,9 @@ func NewDefault() Configuration {
 	defNginxStatusIpv6Whitelist := make([]string, 0)
 	defResponseHeaders := make([]string, 0)

+	defAnnotationValueWordBlocklist := []string{"load_module", "lua_package", "_by_lua", "location", "root"}


should we add others like token etc?

you are fast uh! haha! Yeah, this list should be updated I guess. I tried to add the dangerous ones!

So what list do you think it's good to have here? When you say token, you say like the whole "/var/run/..." for the serviceaccount?

I would also add probably proxy_pass directives, so someone cannot bypass the internal balancer for the evil :D

iamNoah1

Just one little remark :)

iamNoah1 · 2021-11-08T13:14:03Z

test/e2e/admission/admission.go

+		}
+		firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations)
+		_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
+		assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid annotation value should return an error")


Does it make sense to check if the correct err (message) is thrown?

hum, I think we can do that yes. I would leave this anyway as a followup for some issue (feel free to open it!) to improve e2e tests in a sense that we not only verify if the error is not nil, but if error is what we expect (in all tests).

This can become a good first issue, wdyt?

Good Idea, I will open one :)

iamNoah1 · 2021-11-08T13:29:59Z

Just saw one comment that might be related to this PR: kubernetes/kubernetes#126811. Just in case you missed it @rikatz

theunrealgeek · 2021-11-09T02:48:00Z

internal/ingress/controller/controller.go

 		if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
 			if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.DefaultAnnotationsPrefix)) {
 				return fmt.Errorf("This deployment has a custom annotation prefix defined. Use '%s' instead of '%s'", parser.AnnotationsPrefix, parser.DefaultAnnotationsPrefix)
 			}
 		}
+		if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) {
+			for _, forbiddenvalue := range arraybadWords {
+				if strings.Contains(value, forbiddenvalue) {


My only thought is if we need to run strings.TrimSpace every element as well, I’m afraid Contains may fail otherwise.

can you give me one example? not following here (rainy day, a bit sleepy... :) )

Here’s what I mean:
https://play.golang.org/p/mqWmLjoz63N

@theunrealgeek GOOD CATCH!!! Thanks! I will quickly fix this and also add some regression / e2e test before releasing!

But now I see that I do trimming a bit upper:

arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")

Don't this solve the problem?

@rikatz

arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")

will only trim space at the beginning and end of the annotation value, but not in between elements that may exist around the commas which is what my example in the Go playground is pointing out.

In the default value this won’t be a problem since you are coming the CSV with a Join on the list, but for user overridden ones it will be hard to enforce that they don’t put spaces between the commas to make things more readable.

ok, I see :) I was in my head thinking that trimSpace works for all the array, not only beginning and ending and you are right :)

Weirdly, when adding this to unit_test it passes as well, will have to check it better.

Anyway, PTAL here: #7921

theunrealgeek · 2021-11-09T02:49:30Z

internal/ingress/controller/controller_test.go

+				t:   t,
+				err: nil,
+			}
+			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "invalid_directive"


With context of my previous comment, checking for another_directive would also perhaps be useful.

https://github.com/kubernetes/ingress-nginx/pull/7921/files

Added here just to make sure it's working fine :)

tao12345666333 · 2021-11-09T06:13:10Z

internal/ingress/controller/config/config.go

@@ -754,6 +760,20 @@ func NewDefault() Configuration {
 	defNginxStatusIpv6Whitelist := make([]string, 0)
 	defResponseHeaders := make([]string, 0)

+	defAnnotationValueWordBlocklist := []string{


Do we allow users to override the default value? Or should it be appended?

They can override. Appending IMO is bad, as the idea of overriding is actually removing something that may be problematic for them (like, if you use mod_security directives you want to remove {} from the list)

strongjz · 2021-11-09T16:33:19Z

/kind feature
/priority critical-urgent
/triage accepted
;)

* Add option to sanitize annotation inputs * Fix e2e tests after string sanitization * Add proxy_pass and serviceaccount as denied values

* fix: fix thread synchronization issue #6245 (#7800) * Add option to sanitize annotation inputs (#7874) * Add option to sanitize annotation inputs * Fix e2e tests after string sanitization * Add proxy_pass and serviceaccount as denied values * Trim spaces from badword items (#7921) * Fix tests from cherrypick Co-authored-by: Jens Reimann <ctron@dentrassi.de>

* Add option to sanitize annotation inputs * Fix e2e tests after string sanitization * Add proxy_pass and serviceaccount as denied values

Add option to sanitize annotation inputs

f490a4a

k8s-ci-robot requested review from ElvinEfendi and tao12345666333 November 2, 2021 20:50

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2021

strongjz reviewed Nov 2, 2021

View reviewed changes

rikatz added 2 commits November 2, 2021 22:41

Fix e2e tests after string sanitization

c5dd4f1

Add proxy_pass and serviceaccount as denied values

cfa573a

iamNoah1 reviewed Nov 8, 2021

View reviewed changes

theunrealgeek reviewed Nov 9, 2021

View reviewed changes

tao12345666333 reviewed Nov 9, 2021

View reviewed changes

iamNoah1 mentioned this pull request Nov 12, 2021

Just one little remark :) #7910

Closed

rikatz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021

rikatz added this to the v1.0.5 milestone Nov 12, 2021

k8s-ci-robot merged commit 67e13bf into kubernetes:main Nov 12, 2021

strongjz mentioned this pull request Nov 16, 2021

CVE-2021-25742: Ingress-nginx custom snippets allows retrieval of ingress-nginx serviceaccount token and secrets across all namespaces kubernetes/kubernetes#126811

Closed

pierluigilenoci mentioned this pull request Nov 17, 2021

Ingress incompatible with the new version of NGINX ingress controller RasaHQ/rasa-x-helm#246

Closed

dirsigler mentioned this pull request Nov 22, 2021

Admission Webhook denies request dirsigler/uptime-kuma-helm#7

Closed

rittneje mentioned this pull request May 31, 2022

suggested annotation-value-word-blocklist does not match default value from 1.0.5 #8651

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to sanitize annotation inputs #7874

Add option to sanitize annotation inputs #7874

rikatz commented Nov 2, 2021 •

edited

Loading

k8s-ci-robot commented Nov 2, 2021

strongjz Nov 2, 2021

rikatz Nov 2, 2021

rikatz Nov 3, 2021

rikatz Nov 3, 2021

iamNoah1 left a comment

iamNoah1 Nov 8, 2021

rikatz Nov 11, 2021

iamNoah1 Nov 12, 2021

iamNoah1 commented Nov 8, 2021

theunrealgeek Nov 9, 2021

rikatz Nov 11, 2021

theunrealgeek Nov 13, 2021

rikatz Nov 15, 2021

rikatz Nov 15, 2021

theunrealgeek Nov 15, 2021

rikatz Nov 15, 2021

rikatz Nov 15, 2021

theunrealgeek Nov 9, 2021

rikatz Nov 15, 2021

tao12345666333 Nov 9, 2021

rikatz Nov 11, 2021

strongjz commented Nov 9, 2021

Add option to sanitize annotation inputs #7874

Add option to sanitize annotation inputs #7874

Conversation

rikatz commented Nov 2, 2021 • edited Loading

k8s-ci-robot commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamNoah1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamNoah1 commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strongjz commented Nov 9, 2021

rikatz commented Nov 2, 2021 •

edited

Loading